Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RPC] Ignore importdescriptor error #862

Closed
wants to merge 1 commit into from
Closed

[RPC] Ignore importdescriptor error #862

wants to merge 1 commit into from

Conversation

rajarshimaitra
Copy link
Contributor

Description

Fixes #859

The importdescriptor call will throw a Resource temporarily unavialble error only in the first initial sync, where the wallet has to rescan from genesis.

The sync process carries on as usual after the error, so this patch makes it ignore only that particular error and carry on the with sync. All other errors are captured as usual.

Notes to the reviewers

To test this out, sync any descriptor, with the rpc blockchain, in signet/testnet, with a fresh database. This should fail in the current master with the following error.
Rpc(JsonRpc(Transport(SocketError(Os { code: 11, kind: WouldBlock, message: "Resource temporarily unavailable" }))))

This patch will ignore this particular error and carry on with the sync.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • I'm linking the issue being fixed by this PR

w0xlt
w0xlt reviewed Feb 13, 2023
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.
I will test this PR with a full scan.
But I consider this more of a welcome workaround than a solution.

I think it is also important to know the root cause of this error and how to prevent it from occurring.

@notmandatory notmandatory mentioned this pull request Feb 14, 2023
Closed
24 tasks
@rajarshimaitra
Copy link
Contributor Author

I think it is also important to know the root cause of this error and how to prevent it from occurring.

Totally. I will keep digging into this, but the root cause is likely this one: #650 (comment)

@notmandatory notmandatory added the bug Something isn't working label Feb 14, 2023
@notmandatory
Copy link
Member

Concept ACK, needs a rebase.

@wild-kard
Copy link

I have tested this PR as I have been encountering the
bitcoin Rpc(JsonRpc(Transport(SocketError(Os{code: 11, kind: WouldBlock, message: "resource temporarily unavailable"}))))
error when invoking the .sync() method on my bdk wallet.

Ubuntu 22.04.1 LTS
Running a local bitcoin daemon with Bitcoin 23.0-x86_64-linux-gnu

After completing a complete rescan using this build I did not receive any RPC errors and was successfully able to sync.

@danielabrozzoni
Copy link
Member

I don't think this is the right way to handle the problem, as I suppose that this won't import the descriptors in the Bitcoin Core wallet! Also see: https://discord.com/channels/753336465005608961/978744259693916230/1075135066268774462

To the people that tested this PR: did you test with a non-empty wallet, and were you able to see your funds after testing?

@wild-kard
Copy link

I don't think this is the right way to handle the problem, as I suppose that this won't import the descriptors in the Bitcoin Core wallet! Also see: https://discord.com/channels/753336465005608961/978744259693916230/1075135066268774462

To the people that tested this PR: did you test with a non-empty wallet, and were you able to see your funds after testing?

Daniela you are correct. I tested this with a non empty wallet and the funds are not showing properly.

@w0xlt
Copy link
Contributor

w0xlt commented Feb 15, 2023

I think the problem in the PR code is:
. The normal flow in the BDK is to wait for the import/synchronization to finish before continuing the execution.
. With this PR, execution continues when it receives the error, but Bitcoin Core is still importing/synchronizing the wallet.

That makes sense?

@rajarshimaitra
Ignore importdescriptor error
e1229c3 
The importdescriptor call will throw a `Resource temporarily unavialble`
error only in the first initial sync, where the wallet
has to rescan from genesis.

The sync process carries on as usual after the error, so this patch
makes it ignore only that particular error and carry on the with sync.
All other errors are captured as usual.
@rajarshimaitra rajarshimaitra changed the title Ignore importdescriptor error [RPC] Ignore importdescriptor error Feb 16, 2023
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Feb 16, 2023 •
edited
Loading

With this PR, execution continues when it receives the error, but Bitcoin Core is still importing/synchronizing the wallet.

I think what's happening is Bitcoin core throws the error during the import call, and doesn't import all the addresses. So the initial assumption of the PR is wrong.

It seems like this has been addressed in the jsonrpc crate here apoelstra/rust-jsonrpc#72

This should be available in the latest rust-jsonrpc, which is a dep of dep for us. So we need to wait for it to land in the latest bitcoin-core-rpc.

#859 (comment)

In the mean, please feel free to suggest any other possible workaround..

@w0xlt
Copy link
Contributor

w0xlt commented Feb 16, 2023

I tested with jsonrpc = 0.14 locally but the error persisted.

@notmandatory notmandatory mentioned this pull request Feb 27, 2023
Closed
21 tasks
@notmandatory
Copy link
Member

I'm removing this from the next release milestone since it needs more work. Also any fix should be targeted to one of the 1.0.0 alpha releases after #793 is merged, and then if possible back ported to a 0.27.x bug fix release.

@notmandatory notmandatory removed this from the Release 0.27.2 Feature Freeze milestone Mar 3, 2023
@danielabrozzoni
Copy link
Member

Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@w0xlt w0xlt w0xlt left review comments

Assignees

@rajarshimaitra rajarshimaitra

Labels
bug Something isn't working
Projects
BDK Maintenance
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RPC sync failure
5 participants
@rajarshimaitra @notmandatory @wild-kard @danielabrozzoni @w0xlt